-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(jsii): configure outDir and rootDir for tsc #593
Conversation
packages/jsii/lib/project-info.ts
Outdated
@@ -138,6 +140,8 @@ export async function loadProjectInfo(projectRoot: string, { fixPeerDependencies | |||
|
|||
excludeTypescript: (pkg.jsii && pkg.jsii.excludeTypescript) || [], | |||
projectReferences: pkg.jsii && pkg.jsii.projectReferences, | |||
tscOutDir: pkg.jsii && pkg.jsii.tscoutdir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not call it tscOutDir
in the jsii config as well?
(thinking aloud) does this make sense in package.json
or should they be command-line arguments? I guess we're mirroring the tsconfig.json
here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I named it according to the outdir
variable but I am happy to rename it. I think this is something I want to configure for my project. I don't think there is much difference in configuring it inside the jsii
block or in the scripts section ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd like to have options that'll land in tsconfig.json
modeled in a separate namespace altogether:
/// package.json
{
// ...
"jsii": {
// ...
"tsc": {
"outDir": "dist",
"rootDir": "lib",
},
// ...
},
// ...
}
Then, I'd use the exact same name that is in tsconfig
's compilerOptions
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great. I will update this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty good! I have a few minor comments, but I reckon this can be merged once they're addressed!
packages/jsii/lib/compiler.ts
Outdated
|
||
const prog = ts.createProgram({ | ||
rootNames: this.rootFiles.concat(_pathOfLibraries(this.compilerHost)), | ||
options: COMPILER_OPTIONS, | ||
options: {...COMPILER_OPTIONS, outDir: pi.tsc && pi.tsc.outDir, rootDir: pi.tsc && pi.tsc.rootDir}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be (since we decided to use the same name...):
{ ...pi.tsc, ...COMPILER_OPTIONS }
The order is important here - we don't want pi.tsc
to be able to override the required settings we have in COMPILER_OPTIONS
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now this would work, but if someone adds rootDir or outDir to COMPILER_OPTIONS manual overrides would no longer be possible. But I am fine with the change.
packages/jsii/lib/compiler.ts
Outdated
...COMPILER_OPTIONS, | ||
noEmitOnError: false, | ||
outDir: pi.tsc && pi.tsc.outDir, | ||
rootDir: pi.tsc && pi.tsc.rootDir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, here I'd just ...pi.tsc
before ...COMPILER_OPTIONS
.
packages/jsii/lib/compiler.ts
Outdated
@@ -188,11 +197,13 @@ export class Compiler implements Emitter { | |||
lib: COMPILER_OPTIONS.lib && COMPILER_OPTIONS.lib.map(name => name.slice(4, name.length - 5)), | |||
// Those int-enums, we need to output the names instead | |||
module: COMPILER_OPTIONS.module && ts.ModuleKind[COMPILER_OPTIONS.module], | |||
outDir: pi.tsc && pi.tsc.outDir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here :)
This PR adds the possibility to configure the outDir and the rootDir for
tsc
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.